-
Notifications
You must be signed in to change notification settings - Fork 354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Paint status bar after screen enter animation end #5003
Paint status bar after screen enter animation end #5003
Conversation
DROID-216 Paint status bars after transition
This change order should be apply for following screens:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @sabercodic)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt
line 54 at r1 (raw file):
} } LaunchedEffect(isCollapsable) {
I would use Unit
instead.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Scaffolding.kt
line 78 at r1 (raw file):
} } LaunchedEffect(isCollapsable) {
I would use Unit
instead.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt
line 75 at r1 (raw file):
) { val context = LocalContext.current val isCollapsable by remember { mutableStateOf(false) }
This could be removed.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt
line 80 at r1 (raw file):
LaunchedEffect(Unit) { uiCloseAction.collect { onBackClick() } } LaunchedEffect(isCollapsable) {
Could be replaced with Unit
.
Please clarify the PR title. It would also be nice with a brief description. |
284388a
to
978c434
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt
line 54 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
I would use
Unit
instead.
Done.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Scaffolding.kt
line 78 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
I would use
Unit
instead.
Done.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt
line 75 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
This could be removed.
Done.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt
line 80 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Could be replaced with
Unit
.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @Pururun)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r2, all commit messages.
Reviewable status: 2 of 4 files reviewed, 6 unresolved discussions (waiting on @Pururun and @sabercodic)
-- commits
line 2 at r2:
Clarify this commit message
Code quote:
Change paint statusbar time
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt
line 55 at r2 (raw file):
} LaunchedEffect(Unit) { delay(context.resources.getInteger(R.integer.transition_animation_duration).toLong())
I believe we should use a constant instead since it's easier to use in compose and we're moving away from resources for most other similar things (font styling, colors etc).
Code quote:
R.integer.transition_animation_duration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 6 unresolved discussions (waiting on @albin-mullvad and @Pururun)
Previously, albin-mullvad wrote…
Clarify this commit message
Done.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt
line 55 at r2 (raw file):
Previously, albin-mullvad wrote…
I believe we should use a constant instead since it's easier to use in compose and we're moving away from resources for most other similar things (font styling, colors etc).
Done, I
978c434
to
f45e268
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt
line 55 at r3 (raw file):
} LaunchedEffect(Unit) { delay(SCREEN_ENTER_TRANSITION)
Is there no way of triggering a callback when the animation/transition is complete rather using a delay
? It seems like that would make more sense, especially for cases such as disabled animations (in system settings).
If having a proper callback is simplified by using Compose Navigation and the animation/transition support added in 2.7.0
it might be worth skipping this for now.
Code quote:
delay
f45e268
to
4edbe82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 13 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt
line 55 at r3 (raw file):
Previously, albin-mullvad wrote…
Is there no way of triggering a callback when the animation/transition is complete rather using a
delay
? It seems like that would make more sense, especially for cases such as disabled animations (in system settings).If having a proper callback is simplified by using Compose Navigation and the animation/transition support added in
2.7.0
it might be worth skipping this for now.
Done.
f5a17ba
to
b129f9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 16 of 16 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)
900f6a2
to
ef19f56
Compare
ef19f56
to
6022370
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 16 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
6022370
to
438d66c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
438d66c
to
d67ccfb
Compare
Color phone status bar after fragment transition in following screens:
This change is